io.ReadAll(resp.Body) has no size cap. A compromised registry or MITM could serve a multi-gigabyte response and OOM the process. The same pattern repeats inside extractFromTarGz (line 68) and extractFromZip (line 85) where decompressed content is read — a gzip bomb vector.

Suggestion:

const maxDownloadSize = 500 << 20 // 500 MB
body, err := io.ReadAll(io.LimitReader(resp.Body, maxDownloadSize))

And similarly wrap the io.ReadAll(tr) / io.ReadAll(rc) calls in the extract functions.

Two behavioral changes from the old closeClient in run.go worth confirming are intentional:

1. Kill order reversed — old code logged then killed; new code kills first. Probably fine since logging reads PluginPkg fields not client state, but wanted to flag it.

2. "Unexpected exit" guard dropped — the old code had three branches: success (Info), error (Error), and a defensive catch-all for !Successful && Error == nil → Error("unexpected exit..."). The new code only has two branches, so a plugin that finishes without setting either Successful or Error now silently logs Trace("Completed") instead of Error("unexpected exit"). This could mask bugs.

Suggestion — restore the third branch:

func (p *PluginPkg) closeClient(serviceName string, client *hcplugin.Client, logger hclog.Logger) {
	if p.Successful {
		logger.Info(fmt.Sprintf("Plugin for %s completed successfully", serviceName))
	} else if p.Error != nil {
		logger.Error(fmt.Sprintf("Error from %s: %s", serviceName, p.Error))
	} else {
		logger.Warn(fmt.Sprintf("Unexpected exit from %s with no error or success", serviceName))
	}
	client.Kill()
}

Asymmetric trimming — TrimSpace is applied to the registry list entries but not to the user-provided name. If a user passes a name with accidental whitespace (e.g. from copy-paste), the match fails silently.

Additionally, parsePluginName (line 86) does no validation, so input like "../../something" produces owner="../..", repo="something", which gets interpolated into the registry URL path.

Suggestion:

func isVetted(plugins []string, name string) bool {
	name = strings.TrimSpace(name)
	for _, p := range plugins {
		if strings.TrimSpace(p) == name {
			return true
		}
	}
	return false
}

And validate owner/repo in parsePluginName against something like ^[a-zA-Z0-9][a-zA-Z0-9._-]*$.

This function can never return a non-nil error — every code path returns nil. The caller at line 51 checks if err != nil which is dead code. In Go, if a function cannot fail, it should not return an error — the signature misleads readers into thinking validation happens here when none does.

Suggestion: either drop the error return, or add actual validation (empty owner/repo, path traversal characters) so the error return earns its keep.

Not blocking: remote, _ := fetchVettedPlugins() discards the error. When --all is passed and the registry is unreachable, the user sees only local plugins with no indication the remote list failed. Compare with writeInstallablePlugins which does surface the error.

In idiomatic Go, errors should be handled or intentionally logged, not silently discarded. At minimum:

remote, err := fetchVettedPlugins()
if err != nil {
	log.Printf("Warning: could not fetch remote plugin list: %v", err)
}

How would this work if the registry is intentionally unreachable (ie, offline / vpc / airgap) ?

Should we add the warning early, and then handle the alternate use case later?

Or should we add the warning after we have a mechanism for handling intentional isolation?

TODO: either way, we need an issue for the followup

Good call on the airgap case — a warning every time pvtr list -a runs offline would be noisy and annoying in that scenario.

I'd lean toward adding the warning now behind a check — something like: only warn if PVTR_REGISTRY_URL is set (i.e. the user has explicitly configured a registry) or if the default registry was expected to be reachable. Alternatively, a --offline flag or a config option like registry: disabled could suppress it.

But if that feels like scope creep for this PR, creating a follow-up issue works too. The core concern is just that users in connected environments shouldn't get silently partial results with no indication that remote data is missing.

Not blocking: if binaries-path is not configured, viper.GetString returns "". os.MkdirAll("", 0755) succeeds silently, and filepath.Join("", binaryName) resolves to just binaryName — writing the binary to the current working directory with no error.

Suggestion:

destDir := viper.GetString("binaries-path")
if destDir == "" {
	return fmt.Errorf("binaries-path is not configured")
}

Is this the behavior we want? If the user unsets this, it would likely be an intentional unset to target binaries in the working directory.

...Right? 🤔

That's a fair point — I could see an argument either way. My concern is more about the accidental case: a first-time user who hasn't configured anything yet runs pvtr install and silently gets a binary dropped in their cwd. That's a confusing failure mode to debug.

If targeting the working directory is a legitimate use case, maybe make it explicit — e.g. require binaries-path: "." in the config rather than having "" silently resolve to cwd. That way it's an intentional choice, not a footgun. Either way, not blocking.

Not blocking: path.Base(pluginData.Name) strips directory components but does not validate the resulting name. If the registry is compromised, an attacker could control the filename written to the binaries dir (e.g. .bashrc, .profile). Worth validating against a safe pattern like ^[a-zA-Z0-9][a-zA-Z0-9._-]+$ before writing.

Not blocking: writer.Flush() is only called on the success path (line 78). If any error returns early, buffered output like "Fetching metadata..." may be lost. Idiomatic Go would use defer at the top of the function:

Not blocking: this is a breaking change to an exported function signature (Writer → func() Writer). The lazy init rationale is sound, just flagging that downstream consumers will need a coordinated update. The PR description mentions privateer#204 which likely handles this.

Not blocking: --installable, --installed, and --all are not mutually exclusive. If a user passes --installable --installed, installable silently wins. Cobra supports cmd.MarkFlagsMutuallyExclusive("installable", "installed", "all") which would give a clear error message instead.